Skip to content

Conversation

@ignavierng
Copy link
Contributor

@ignavierng ignavierng commented Jul 13, 2022

Updates

  • Added test cases for Astar and DP
  • Added simulated data and ground truth for the test cases
  • Removed previous test cases for Astar and DP

Test Plan

python -m unittest tests.TestAstar # should pass
python -m unittest tests.TestDP # should pass

TestAstar
TestDP

Copy link
Contributor

@tofuwen tofuwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome work!

Only some very nits need to be addressed. :)

def test_astar_simulate_linear_gaussian_with_local_score_BIC(self):
print('Now start test_astar_simulate_linear_gaussian_with_local_score_BIC ...')
truth_CPDAG_matrix = np.loadtxt("./TestData/test_exact_search_simulated_linear_gaussian_CPDAG.txt")
data = np.loadtxt("./TestData/test_exact_search_simulated_linear_gaussian_data.txt")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we generate this data? Do you mind adding the data generating script in this file as well (but comment out the code) --- this can ensure we know how the data is generated later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I have added and commented the code to generate the data and ground truth.

@tofuwen
Copy link
Contributor

tofuwen commented Jul 13, 2022

In your screenshot, I saw 0,1,2,3 are printed. Do we need to print them? Maybe we can remove it?

@ignavierng
Copy link
Contributor Author

ignavierng commented Jul 13, 2022

In your screenshot, I saw 0,1,2,3 are printed. Do we need to print them? Maybe we can remove it?

I notice that the numbers are printed when the function dag2cpdag is called in the test, i.e., CPDAG = dag2cpdag(DAG). Perhaps that would require modifying the function dag2cpdag.

@tofuwen
Copy link
Contributor

tofuwen commented Jul 13, 2022

Yeah, please change the dag2cpdag. We don't the need the extra print in test, which is confusing.

And in any industry level code, there shouldn't be print statement haha

@ignavierng
Copy link
Contributor Author

Yeah, please change the dag2cpdag. We don't the need the extra print in test, which is confusing.

And in any industry level code, there shouldn't be print statement haha

I notice that the reason is not the dag2cpdag function. The issue was that the tests I wrote were importing causallearn from my pip installed packages instead of the local code, which was not desired. I have fixed it. Thanks!

Copy link
Contributor

@tofuwen tofuwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! Thanks for the great work!

cc @kunwuz to merge this

@kunwuz kunwuz merged commit 129dcdf into py-why:main Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants